Skip to content

Conversation

@lifeizhou-ap
Copy link
Collaborator

@lifeizhou-ap lifeizhou-ap commented Aug 11, 2025

Implement the feature for #3724

What

  • Added the configuration for users to choose whether to load the nested goose hint file
  • When configuration is enabled, it will load the hints file up to the directory with .git dir or current directory if no .git found

Next (not in this PR)
It would be good to show loaded hints in CLI and Desktop.

let hints_filenames: Vec<String> = std::env::var("CONTEXT_FILE_NAMES")
.ok()
.and_then(|s| serde_json::from_str(&s).ok())
.unwrap_or_else(|| vec![".goosehints".to_string()]);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted the hints logic to goose_hints.rs

@lifeizhou-ap lifeizhou-ap requested review from DOsinga and michaelneale and removed request for DOsinga August 11, 2025 11:05
* main:
  feat: add @-mention file reference expansion to .goosehints (#3873)
  feat(cli): Add --name/-n to session remove and --id/-i alias for session export (#3941)
  Docs: provider and model run options (#4013)
  To-Do Tools (#3902)
  ci: correctly match doc only changes (#4009)
  Remove PR trigger for Linux build workflow (#4008)
  docs: update release docs with an additional step needed + adjust list formatting (#4005)
  chore(release): release version 1.3.0 (#3921)
  docs: MCP-ui blog content (#3996)
  feat: Add `GOOSE_TERMINAL` env variable to spawned terminals (#3911)
  add missing dependencies for developer setup (#3930)
let _ = cliclack::log::info("Notice: GOOSE_NESTED_HINTS environment variable is set and will override the configuration here.");
}

let current_enabled: bool = config.get_param("NESTED_GOOSE_HINTS").unwrap_or(false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think enabled could be a better default here, and people would only need to seek out the config option if they didn't want it for some reason.

.expect("Invalid file reference regex pattern")
});

const MAX_DEPTH: usize = 3;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why three? I'd think there would be realistic repo scenarios of wanting four or five perhaps

Copy link
Collaborator Author

@lifeizhou-ap lifeizhou-ap Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is copied from existing behaviour during refactoring.

This max depth is more about reference depth. For example,

file 1
@file2.md

file2
@file3.md

file3
@file4.md

We can increase if we would like to have more chain of references

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh i see

if name == "developer" {
let config = crate::config::Config::global();
if let Ok(nested_enabled) = config.get_param::<bool>("NESTED_GOOSE_HINTS") {
command.env("NESTED_GOOSE_HINTS", nested_enabled.to_string());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we not already forward env vars to mcp server subprocesses?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we get the NESTED_GOOSE_HINTS value from config, and pass it to the mcp as environment variable as it is not good for mcp server to know the config.

We could simplify this without make this behaviour configuration as suggested in the comments below

};
use rmcp::object;

use crate::developer::goose_hints::load_hints::{load_hint_files, GOOSE_HINTS_FILENAME};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to see developer get smaller and this to be moved out!

I do wonder if we will need tighter integration again soon though. It could be nice to have it discover .goosehints in subdirs when working with files in those subdirs. And feels like the depth limit could be more relaxed for this case...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this is a slightly different requirements

In the implementation of this PR, while working on the current directly, it inherits all the .goosehints from the parent directory up to the project root.

It could be nice to have it discover .goosehints in subdirs when working with files in those subdirs. And feels like the depth limit could be more relaxed for this case...

This would be also useful. This also aligns the glob features in cursor, and Claude code has something similar. We can work on this separately

@alexhancock
Copy link
Collaborator

Tried it locally and asked:

"What .goosehints do you already know about?" from cwd of goose/ui/desktop

it picked up the one from goose/ui/desktop/.goosehints but not goose/.goosehints

I wonder if this should work, or indicates an issue with the upward traversal

@michaelneale
Copy link
Collaborator

I think consider default on for this, if it isn't ready for that, then it needs more work IMO. This would use the nearest hints - is that right? or does it keep loading them all the way up?

@alexhancock
Copy link
Collaborator

Yeah I agree with @michaelneale above! I think this would be good just "default to on" behavior.

I also don't know that it needs to be configureable tbh. The more options we add to settings the more conceptual overhead there is for the user. People could configure it by modifying/adjusting/removing the content of the goosehints files it finds and uses.

* main: (108 commits)
  Remove unused game (#4226)
  fix issue where app redirects to home after initialization but user has already started a chat (#4260)
  Feat: Let providers configure a fast model for summarization (#4228)
  docs: update tool selection strategy (#4258)
  feat: upgrade `@mcp-ui/client` package and improve UI message handling (#4164)
  stop replacing chat window when changing working directory (#4200)
  Only fetch session tokens when chat state is idle to avoid resetting during streaming (#4104)
  bump timeouts for e2e tests (#4251)
  docs: custom context files improvements (#4096)
  chore: upgrade rmcp to 0.6.0 (#4243)
  doc: uvx not npx (#4240)
  Add PKCE support for Tetrate Agent Router Service (#4165)
  Read AGENTS.md by default (#4232)
  docs: configure provider and model (#4235)
  docs: add figma tutorial (#4231)
  Add Nix flake for reproducible builds (#4213)
  Enhanced onboarding page visual design (#4156)
  feat: adds mtls to all providers (#2794) (#2799)
  Don't show a confirm dialog for quitting (#4225)
  Fix: Missing smart_approve in CLI /mode help text and error message (#4132)
  ...
@lifeizhou-ap
Copy link
Collaborator Author

lifeizhou-ap commented Aug 22, 2025

I think consider default on for this, if it isn't ready for that, then it needs more work IMO.

👍

This would use the nearest hints - is that right? or does it keep loading them all the way up?

It will keep loading them all the way up until they see the directory with .git (eg project root). Otherwise, it uses the .goosehints in the current directory

@lifeizhou-ap
Copy link
Collaborator Author

Yeah I agree with @michaelneale above! I think this would be good just "default to on" behavior.

I also don't know that it needs to be configureable tbh. The more options we add to settings the more conceptual overhead there is for the user. People could configure it by modifying/adjusting/removing the content of the goosehints files it finds and uses.

👍 This totally makes sense!

@lifeizhou-ap
Copy link
Collaborator Author

Tried it locally and asked:

"What .goosehints do you already know about?" from cwd of goose/ui/desktop

it picked up the one from goose/ui/desktop/.goosehints but not goose/.goosehints

I wonder if this should work, or indicates an issue with the upward traversal

Hi @alexhancock Thank you so much for testing this! Really appreciate it!

I tried on local, it seems Goose picked up the goose/.goosehints. I guess it might need to run the developer mcp to load the hints. (this is the existing behaviour).

Screenshot 2025-08-22 at 12 21 21 pm

@lifeizhou-ap
Copy link
Collaborator Author

lifeizhou-ap commented Aug 22, 2025

Hi @alexhancock @michaelneale,

Thank you so much for your time to review this PR. I've replied to your comments and removed the NESTED_GOOSE_HINTS configuration as suggested.

I added a bit clarification on MAX_DEPTH, happy to increase if you think 4/5 depth will be helpful.

Thank you!

Copy link
Collaborator

@alexhancock alexhancock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So much simpler without the config options. This makes sense as default behavior to me. Nice!

@lifeizhou-ap lifeizhou-ap merged commit 298fb90 into main Aug 26, 2025
11 checks passed
@lifeizhou-ap lifeizhou-ap deleted the lifei/read-upper-goosehints branch August 26, 2025 00:51
shellz-n-stuff pushed a commit to shellz-n-stuff/goose that referenced this pull request Aug 27, 2025
Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
lifeizhou-ap added a commit that referenced this pull request Aug 29, 2025
* main:
  no streaming for ollama and re-arrange cards (#4335)
  Enable load nested hint files (#4002)
dorien-koelemeijer pushed a commit to dorien-koelemeijer/goose that referenced this pull request Sep 2, 2025
Signed-off-by: Dorien Koelemeijer <dkoelemeijer@squareup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants